Python: Change note for new points-to implementation.#1425
Conversation
felicitymay
left a comment
felicitymay
left a comment
There was a problem hiding this comment.
Thanks. It's great to have more information about the changes in 1.21. A couple of questions and some suggestions.
|
|
||
| * Non-local tracking of bound methods and instances of `super()` | ||
| * Superior analysis of conditionals and thus improved reachability analysis. | ||
| * Superior modelling of descriptors, for example, classmethods and staticmethods. |
There was a problem hiding this comment.
modelling -> modeling
Are "classmethods" and "staticmethods" code elements, or should they each be two words?
There was a problem hiding this comment.
They are code elements
|
|
||
| ### Impact on existing queries. | ||
|
|
||
| As points-to underpins many queries and provides the call-graph and reachability analysis required for taint-tracking, many queries will have additional results and some may have fewer results. |
There was a problem hiding this comment.
Suggest: "As points-to analysis underpins many queries, and provides the call-graph and reachability analysis required for taint-tracking, the results of many queries will change.
|
|
||
| As points-to underpins many queries and provides the call-graph and reachability analysis required for taint-tracking, many queries will have additional results and some may have fewer results. | ||
|
|
||
| New results are a result of the improved reachability analysis and non-local tracking of bound-methods. |
There was a problem hiding this comment.
To avoid using "result" in two different senses, perhaps: "The improved reachability analysis and non-local tracking of bound methods will identify new results."?
|
|
||
| New results are a result of the improved reachability analysis and non-local tracking of bound-methods. | ||
| Removed results are a result of more precise tracking of values through `*` arguments. | ||
| The expectation is that number of true positives will increase and the number of false negatives will decline. |
There was a problem hiding this comment.
Perhaps: "Overall the number of true positive results should increase and the number false negative results should decline. " ?
| New results are a result of the improved reachability analysis and non-local tracking of bound-methods. | ||
| Removed results are a result of more precise tracking of values through `*` arguments. | ||
| The expectation is that number of true positives will increase and the number of false negatives will decline. | ||
| However, this is new code and may still contain errors. |
There was a problem hiding this comment.
What are we trying to say here? That the API will be developed further, that it's a preliminary version, that we haven't tested it, that we welcome feedback on it...?
|
@felicity-semmle I've addressed your comments. I've used "may" rather than "will" throughout, as we can't find more results that aren't there to be found, nor fix false positives that don't exist. |
felicitymay
left a comment
felicitymay
left a comment
There was a problem hiding this comment.
Thanks for the changes. All LGTM now.
2 participants
The missing change note for #1277 and subsequent related PRs.